Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix env transforms in node_modules #2073

Closed
wants to merge 1 commit into from
Closed

Conversation

DeMoorJasper
Copy link
Member

@DeMoorJasper DeMoorJasper commented Sep 27, 2018


name: 🙋 New Pull Request
about: Do you want to change something on parcel?

↪️ Pull Request

I'm not sure if this is what should happen, as there where 2 unit tests stating that the modules should not be converted to match the global browser versions. Which seems kinda strange to me as that is the only reason preset-env is being ran over node_modules afaik?

This PR, removes the fallback to engines.node for target detection in case of targeting browsers as this is often an invalid indicator for browser specific modules.

Closes #1887

💻 Examples

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions
  • Included links to related issues/PRs

…ges and don't dedupe env plugins on unknown environments
@DeMoorJasper DeMoorJasper changed the title don't rely on node engines to be reliable reference for browser packa… Fix env transforms in node_modules Sep 27, 2018
@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Sep 27, 2018

Apparently the linux test-runners are down.

The scope-hoisting tests are using experimental JS in some tests so babel is complaining. I'll try to find some time soonish to dig deeper into fixing that.

I just hope nobody publishes code with unstandardized JS, otherwise this would be a breaking change.

@fathyb you have any idea if this would affect scope-hoisting a lot? And whether this PR is even a good idea at all.

@DeMoorJasper DeMoorJasper requested a review from fathyb September 28, 2018 17:51
@fathyb
Copy link
Contributor

fathyb commented Sep 29, 2018

As far as scope-hoisting goes I think it'll be okay as soon as it doesn't add the commonjs transform.
For the rest I'm not really sure, it makes sense to me but I'm not sure of the consequences. The tests are coming from #559, pinging @devongovett for advice!

@devongovett
Copy link
Member

Hmm I don't know that we want to change this. We currently assume that node_modules is already compiled by default, unless a browserslist or engines.node value indicates otherwise. Without either of those keys in package.json, I'm not sure how we could assume anything about how the code was written other than that it is already compiled. Turning on babel for all node_modules will slow down Parcel by a ton I would guess.

@DeMoorJasper
Copy link
Member Author

@devongovett originally thought the logic was assuring the entire bundle matches a certain browserslist. This is the only way I could think of achieving that as defining a browserslist or engines field in a package is optional.

Perhaps we could allow some way to enable this, for people who really want full browser compatibility?

Not sure if this is a good idea or not, seems to break the moment anything has unstandardised JS.

@devongovett
Copy link
Member

node_modules shouldn't contain non-standard code for sure, but historically node_modules has been ES5 as well, meaning pre-babelified. This ensures the most interoperability between tools. Parcel takes this as the default, and assumes that if no other information is specified, the code is already pre-compiled to ES5. However, people are starting to target newer versions of node/browsers in their node_modules, so we added support for specifying that in engines.node and browserslist fields. When those are specified, and they target higher versions than the app, we run babel over the module. Otherwise, babel is not run over node_modules.

@DeMoorJasper
Copy link
Member Author

Alright so people should just change their package.json, got it.

@n0v1
Copy link

n0v1 commented Mar 13, 2019

I just stumbled upon are-you-es5 which tries to parse each dependency with acorn to tell if it is valid es5 or not. I guess Parcel could check dependencies before bundling or the generated bundles with this or a similar approach and transpile them in case they do not fit the target environment. Maybe behind a flag for performance reasons.

Update:

es-check does the same but is more popular.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

production minify code doesn't work well(npm run build), but development is ok(npm start)
4 participants